8362208: [8u] Buffer overflow in g1GCPhaseTimes.cpp::LineBuffer::_buffer #668

sendaoYan · 2025-07-15T06:47:42Z

Hi all,

When running hotspot/test/gc/g1/TestG1TraceEagerReclaimHumongousObjects.java on a CPU with more than 200 physical threads, the jvm will crashes. The reason is that the testcase turn on the gc log, which prints the statistics of each gc thread. If the machine has more cores, more gc threads will be turned on (143 gc threads on a machine with 224 physical threads). In the G1GCParPhasePrinter::print_time_values function (hotspot/src/share/vm/gc_implementation/g1/g1GCPhaseTimes.cpp), the relevant statistics of all gc threads are concatenated into one line, and the string concatenation content is saved in the array defined by g1GCPhaseTimes.cpp::LineBuffer::_buffer. Therefore, on machines with a large number of physical threads, it is easy for the GC log output line length to exceed the predefined buffer size. When the buffer size is exceeded, an error occurs when calling the os::vsnprintf function.
In JDK9, JDK-8150068 refactors the relevant GC log output, so buffer overflow will no longer occur. However, JDK-8150068 is a new feature, and JDK-8150068 cannot be directly backported to jdk8u. In addition, the amount of JDK-8150068 code is large, and the risk of backporting to jdk8u is also very high. Therefore, this PR changes the buffer length to 1024*3 to ensure that there will be no problems with GC log output in some scenarios, and leave a certain margin.

In addition, this PR adds a guarantee statement to ensure that an error is reported before calling os::vsnprintf when the buffer overflows, which is conducive to the rapid location of the problem

Change has been verified locally, risk is low.

Additional testing:

jtreg tests include tier1/2/3 etc.. on linux-x64 with release build

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
JDK-8362208 needs maintainer approval
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8362208: [8u] Buffer overflow in g1GCPhaseTimes.cpp::LineBuffer::_buffer (Bug - P4 - Approved)

Reviewers

Paul Hohensee (@phohensee - Reviewer)
Thomas Stuefe (@tstuefe - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/668/head:pull/668
$ git checkout pull/668

Update a local copy of the PR:
$ git checkout pull/668
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/668/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 668

View PR using the GUI difftool:
$ git pr show -t 668

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/668.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-07-15T06:48:24Z

👋 Welcome back syan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-07-15T06:48:26Z

@sendaoYan This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8362208: [8u] Buffer overflow in g1GCPhaseTimes.cpp::LineBuffer::_buffer

Reviewed-by: phh, stuefe

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

mlbridge · 2025-07-15T06:51:57Z

Webrevs

phohensee

Afaik a guarantee is supposed to check for JVM-ending situations, so it seems to strong to me just for losing some logging info. Even an assert is imo too strong. I'd turn the guarantee into a debug-only warning with something like "previous LineBuffer overflow, request ignored" and have it execute just once. I'd also gate the whole method on (_cur < BUFFER_LEN) so vnsprintf isn't called unnecessarily.

…previous LineBuffer overflow, request ignored"

sendaoYan · 2025-07-17T06:31:21Z

Afaik a guarantee is supposed to check for JVM-ending situations, so it seems to strong to me just for losing some logging info. Even an assert is imo too strong. I'd turn the guarantee into a debug-only warning with something like "previous LineBuffer overflow, request ignored" and have it execute just once. I'd also gate the whole method on (_cur < BUFFER_LEN) so vnsprintf isn't called unnecessarily.

@phohensee Thanks for your suggestions. The guaratee has been removed, and I add a debug only warning, and then return early when _cur > BUFFER_LEN

sendaoYan · 2025-07-17T09:30:45Z

GHA report several test failures:

Several 'jdk/security_infra' job fails, it's known CA test issue, it's unrelated to this PR.

phohensee

Looks like the test on line 44 should be

_cur >= BUFFER_LEN

because _cur is set to BUFFER_LEN at line 52 as the sentinal.

sendaoYan · 2025-07-18T06:04:52Z

Looks like the test on line 44 should be

_cur >= BUFFER_LEN

because _cur is set to BUFFER_LEN at line 52 as the sentinal.

Thanks for your correction. PR has been updated.

phohensee

Thanks, looks good.

openjdk · 2025-07-18T16:55:25Z

⚠️ @sendaoYan This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

sendaoYan · 2025-07-19T02:54:29Z

Thanks, looks good.

Thanks for the reviews @phohensee

/approval request Fix buffer overflow in g1GCPhaseTimes.cpp::LineBuffer::_buffer, change has been verified by full jtreg tests.

openjdk · 2025-07-19T02:54:47Z

@sendaoYan
8362208: The approval request has been created successfully.

bridgekeeper · 2025-08-16T04:27:54Z

@sendaoYan This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

sendaoYan · 2025-08-18T02:34:15Z

/touch

openjdk · 2025-08-18T02:35:50Z

@sendaoYan The pull request is being re-evaluated and the inactivity timeout has been reset.

jerboaa · 2025-08-27T14:52:37Z

Please get a second review for this 8u-only change as is custom for Hotspot changes.

sendaoYan · 2025-08-28T02:11:09Z

/reviewer 2

openjdk · 2025-08-28T02:11:59Z

@sendaoYan Syntax: /reviewer (credit|remove) [@user | openjdk-user]+. For example:

/reviewer credit @openjdk-bot
/reviewer credit duke
/reviewer credit @user1 @user2

sendaoYan · 2025-08-28T02:12:57Z

Looking the 2rd reviewers for this PR.

jerboaa · 2025-08-28T08:38:31Z

/reviewers 2

jerboaa · 2025-08-28T08:38:54Z

@tstuefe Could you perhaps help get this reviewed? Thanks!

openjdk · 2025-08-28T08:39:33Z

@jerboaa
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

tstuefe

Is this Linebuffer object stack allocated or is it (part of another) heap allocated (object)?

If the latter, I would increase the buffer size further.

sendaoYan · 2025-08-28T09:30:46Z

Is this Linebuffer object stack allocated or is it (part of another) heap allocated (object)?

If the latter, I would increase the buffer size further.

It's "Linebuffer object stack allocated". It used for gc log concatenation

bridgekeeper · 2025-09-25T13:33:54Z

@sendaoYan This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

sendaoYan · 2025-09-25T14:39:58Z

Looking for the 2rd reviewer for this bug fix. The buffer is used for gc log concatenation

tstuefe

Ok.

sendaoYan · 2025-09-26T01:58:50Z

Thanks for the suggestions and reviews @phohensee @tstuefe

GHA report two jtreg tests security/infra/java/security/cert/CertPathValidator/certification/CAInterop.java#entrustrootcag4 security/infra/java/security/cert/CertPathValidator/certification/CAInterop.java#sslrootevrsaca fails. I think it's unrelated to this PR.

/approval request Fix the bug which may cause buffer overflow when there are many gc thread numbers. Change has been verified with jtreg tier 1/2/3. Risk is low.

openjdk · 2025-09-26T02:00:39Z

@sendaoYan
8362208: The approval request has been updated successfully.

sendaoYan · 2025-09-26T09:31:25Z

Thanks for the approved @jerboaa

/integrate

openjdk · 2025-09-26T09:32:11Z

Going to push as commit 7bbd26b.

openjdk · 2025-09-26T09:32:17Z

@sendaoYan Pushed as commit 7bbd26b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot added the rfr Pull request is ready for review label Jul 15, 2025

8362208: [8u] Buffer overflow in g1GCPhaseTimes.cpp::LineBuffer::_buffer

a207869

phohensee reviewed Jul 16, 2025

View reviewed changes

Remove guarantee and add debug only warning, then return early when "…

a132e0c

…previous LineBuffer overflow, request ignored"

fix bugs

55e4fe2

phohensee suggested changes Jul 17, 2025

View reviewed changes

Should be _cur >= BUFFER_LEN

811ce78

phohensee approved these changes Jul 18, 2025

View reviewed changes

openjdk bot added the approval Requires approval; will be removed when approval is received label Jul 19, 2025

openjdk bot removed the approval Requires approval; will be removed when approval is received label Aug 27, 2025

tstuefe reviewed Aug 28, 2025

View reviewed changes

Merge branch 'openjdk:master' into jbs8362208

5b61629

tstuefe approved these changes Sep 25, 2025

View reviewed changes

openjdk bot added approval Requires approval; will be removed when approval is received ready Pull request is ready to be integrated and removed approval Requires approval; will be removed when approval is received labels Sep 26, 2025

openjdk bot added the integrated Pull request has been integrated label Sep 26, 2025

openjdk bot closed this Sep 26, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 26, 2025

sendaoYan deleted the jbs8362208 branch September 26, 2025 09:33

8362208: [8u] Buffer overflow in g1GCPhaseTimes.cpp::LineBuffer::_buffer #668

8362208: [8u] Buffer overflow in g1GCPhaseTimes.cpp::LineBuffer::_buffer #668

Uh oh!

Conversation

sendaoYan commented Jul 15, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jul 15, 2025

Uh oh!

openjdk bot commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

phohensee left a comment

Choose a reason for hiding this comment

Uh oh!

sendaoYan commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sendaoYan commented Jul 17, 2025

Uh oh!

phohensee left a comment

Choose a reason for hiding this comment

Uh oh!

sendaoYan commented Jul 18, 2025

Uh oh!

phohensee left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jul 18, 2025

Uh oh!

sendaoYan commented Jul 19, 2025

Uh oh!

openjdk bot commented Jul 19, 2025

Uh oh!

bridgekeeper bot commented Aug 16, 2025

Uh oh!

sendaoYan commented Aug 18, 2025

Uh oh!

openjdk bot commented Aug 18, 2025

Uh oh!

jerboaa commented Aug 27, 2025

Uh oh!

sendaoYan commented Aug 28, 2025

Uh oh!

openjdk bot commented Aug 28, 2025

Uh oh!

sendaoYan commented Aug 28, 2025

Uh oh!

jerboaa commented Aug 28, 2025

Uh oh!

jerboaa commented Aug 28, 2025

Uh oh!

openjdk bot commented Aug 28, 2025

Uh oh!

tstuefe left a comment

Choose a reason for hiding this comment

Uh oh!

sendaoYan commented Aug 28, 2025

Uh oh!

bridgekeeper bot commented Sep 25, 2025

Uh oh!

sendaoYan commented Sep 25, 2025

Uh oh!

tstuefe left a comment

Choose a reason for hiding this comment

Uh oh!

sendaoYan commented Sep 26, 2025

Uh oh!

openjdk bot commented Sep 26, 2025

Uh oh!

sendaoYan commented Sep 26, 2025

Uh oh!

sendaoYan commented Jul 15, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jul 15, 2025 •

edited

Loading

mlbridge bot commented Jul 15, 2025 •

edited

Loading

sendaoYan commented Jul 17, 2025 •

edited

Loading